Automated Test: plugins/rename-instrumentation-middleware-to-metrics-middleware #328
Conversation
…are (#76186) * Plugins: Chore: Renamed instrumentation middleware to metrics middleware * Removed repeated logger attributes in middleware and contextual logger * renamed loggerParams to logParams * PR review suggestion * Add contextual logger middleware * Removed unused params from logRequest * Removed unwanted changes * Safer FromContext method * Removed traceID from logParams
📝 WalkthroughWalkthroughThis PR introduces context-based logging for plugin operations by adding FromContext methods across logger implementations, updating variadic parameters from interface{} to any, creating a new ContextualLoggerMiddleware that enriches request contexts with logging attributes, refactoring LoggerMiddleware to use context-aware logging instead of per-plugin metadata, and renaming InstrumentationMiddleware to MetricsMiddleware within the middleware chain. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContextualLoggerMiddleware as Contextual Logger<br/>Middleware
participant LoggerMiddleware
participant MetricsMiddleware
participant PluginClient
participant Logger
participant Context as Request<br/>Context
Client->>ContextualLoggerMiddleware: Request (endpoint, pluginId, etc.)
ContextualLoggerMiddleware->>Context: Inject contextual attributes<br/>(endpoint, pluginId, dsName, dsUID, uname)
ContextualLoggerMiddleware->>LoggerMiddleware: QueryData/CallResource/etc<br/>(with enriched context)
LoggerMiddleware->>MetricsMiddleware: Execute request<br/>(context)
MetricsMiddleware->>PluginClient: Execute operation<br/>(context)
PluginClient-->>MetricsMiddleware: Response
MetricsMiddleware-->>LoggerMiddleware: Response
LoggerMiddleware->>Logger: logger.FromContext(ctx).Info<br/>(read attributes from context)
Logger-->>LoggerMiddleware: Log entry with contextual data
LoggerMiddleware-->>ContextualLoggerMiddleware: Response
ContextualLoggerMiddleware-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/plugins/log/fake.go`:
- Around line 46-48: FromContext currently returns a brand-new TestLogger,
disconnecting it from the original logger and breaking assertions; change
TestLogger.FromContext to return the receiver (retain the original TestLogger's
context) instead of NewTestLogger() so LoggerMiddleware calls like
m.logger.FromContext(ctx).Info(...) operate on the same TestLogger instance
(refer to TestLogger.FromContext and LoggerMiddleware).
In
`@pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go`:
- Around line 39-57: The four middleware methods (QueryData, CallResource,
CheckHealth, CollectMetrics) currently dereference req.PluginContext without
checking req for nil; update each method to first check if req == nil and only
call instrumentContext with req.PluginContext when req is non-nil (otherwise
call instrumentContext with a nil/empty plugin context or leave ctx unchanged),
then forward the call to m.next with the original req and parameters; ensure you
reference and modify the functions QueryData, CallResource, CheckHealth, and
CollectMetrics in contextual_logger_middleware.go.
🧹 Nitpick comments (2)
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
21-21: Stale test function name after rename.The type under test was renamed to
MetricsMiddleware, but the test function is stillTestInstrumentationMiddleware. Consider renaming it toTestMetricsMiddlewarefor consistency and discoverability.pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (1)
75-82: Nit:imwvariable name is a leftover from the oldInstrumentationMiddlewarenaming.Consider renaming
imwtomwormmwto align with the newMetricsMiddlewaretype name.♻️ Suggested rename
func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) plugins.ClientMiddleware { - imw := newMetricsMiddleware(promRegisterer, pluginRegistry) + mw := newMetricsMiddleware(promRegisterer, pluginRegistry) return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { - imw.next = next - return imw + mw.next = next + return mw }) }
| func (f *TestLogger) FromContext(_ context.Context) Logger { | ||
| return NewTestLogger() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the fake.go file around the mentioned lines
cd /tmp && find . -name "fake.go" -type f 2>/dev/null | head -20Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Let's find the file structure
find . -name "*.go" -path "*/log/*" -type f 2>/dev/null | head -20Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 716
🏁 Script executed:
# Search for FromContext method implementations
rg -n "func.*FromContext" --type=go -A 3Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 4484
🏁 Script executed:
# Look for LoggerMiddleware and its usage of FromContext
rg -n "LoggerMiddleware\|FromContext" --type=go -B 2 -A 2Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Check for test files that might assert on log fields
rg -n "InfoLogs|DebugLogs|WarnLogs|ErrorLogs" --type=go | head -30Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1972
🏁 Script executed:
# Look for logger_middleware.go
find . -name "*logger*middleware*" -o -name "*middleware*logger*" | grep -E "\.go$"Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 381
🏁 Script executed:
# Check the Logger interface definition in plugins/log
cat pkg/plugins/log/ifaces.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1329
🏁 Script executed:
# Look for tests in plugins/log directory
ls -la pkg/plugins/log/Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 444
🏁 Script executed:
# Check the fake.go file completely
cat pkg/plugins/log/fake.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1775
🏁 Script executed:
# Check if logtest/fake.go and plugins/log/fake.go have same behavior
echo "=== pkg/infra/log/logtest/fake.go ===" && sed -n '66,68p' pkg/infra/log/logtest/fake.go
echo "=== pkg/plugins/log/fake.go ===" && sed -n '46,48p' pkg/plugins/log/fake.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 319
🏁 Script executed:
# Search for tests that might use plugins/log fake logger
find pkg/plugins -name "*test.go" -type fRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1501
🏁 Script executed:
# Check if any tests specifically test FromContext behavior
rg -n "FromContext" pkg/plugins --type=go -B 3 -A 3Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1390
🏁 Script executed:
# Check the logger_middleware.go file to see if it uses plugins/log
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 3623
🏁 Script executed:
# Check if there are tests for this middleware
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware_test.go 2>/dev/null | head -100Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Search for tests in plugins directory that use TestLogger and FromContext
rg -n "TestLogger.*FromContext\|FromContext.*TestLogger" pkg/plugins --type=go -B 3 -A 3Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Check the infra logger implementation to see how it handles FromContext
cat pkg/infra/log/log.go | sed -n '203,220p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 431
🏁 Script executed:
# Check for any tests that use FromContext with fake loggers and assert on logs
rg -n "FromContext" pkg/plugins/manager/pipeline/bootstrap/steps_test.go -B 5 -A 5Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Check if there's a test for LoggerMiddleware
find pkg/services/pluginsintegration -name "*logger_middleware*test*" -type fRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Look at the test file if it exists
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware_test.go 2>/dev/null || echo "File not found"Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 100
🏁 Script executed:
# Search for any tests that instantiate TestLogger from plugins/log
rg -n "plugins.*log.*TestLogger\|log\.TestLogger" pkg --type=go -B 2 -A 5Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Check how plugins/log is actually used in tests vs how infra/log/logtest is used
rg -n "from.*log\.TestLogger\|plugins/log" pkg/plugins/manager/pipeline/bootstrap/steps_test.go -B 3 -A 3Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Let's see the actual bootstrap steps_test.go which we know uses fake loggers
cat pkg/plugins/manager/pipeline/bootstrap/steps_test.go | head -100Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 3010
🏁 Script executed:
# Check if LoggerMiddleware is actually used in tests and if tests would fail
rg -n "LoggerMiddleware" pkg/services/pluginsintegration --type=go -B 2 -A 2Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 12730
🏁 Script executed:
# Verify the complete picture: what does the real infra logger's New() method do?
cat pkg/infra/log/log.go | sed -n '213,230p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 513
🏁 Script executed:
# Compare the interface semantics - what should FromContext really do?
cat pkg/plugins/log/ifaces.go | sed -n '20,24p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 263
🏁 Script executed:
# Check if there are any integration tests that would catch this issue
find pkg/services/pluginsintegration -name "*.go" -type f | xargs grep -l "TestLogger\|LoggerMiddleware" | head -5Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 403
🏁 Script executed:
# Check if there are any tests that actually use TestLogger with LoggerMiddleware or similar FromContext calls
rg -n "TestLogger\|NewTestLogger" pkg/services/pluginsintegration --type=goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
# Let's look at the contextual_logger_middleware to see if it also calls FromContext
cat pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 2842
🏁 Script executed:
# Final verification: check if the infra logtest fake's behavior is consistent with the real logger
cat pkg/infra/log/logtest/fake.go | sed -n '1,80p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1300
FromContext returns a disconnected TestLogger, breaking potential test assertions through LoggerMiddleware.
The middleware at line 60 of logger_middleware.go calls m.logger.FromContext(ctx).Info(...). Since FromContext returns a brand-new TestLogger instance unrelated to the one tests hold a reference to, any assertions on InfoLogs.Calls, InfoLogs.Message, etc. will never capture logs written through the middleware. The interface specifies that FromContext should return "a new contextual Logger that has this logger's context"—meaning it should retain context from the original logger, not disconnect entirely. While no tests currently use TestLogger with this middleware (only production uses real loggers), this will silently break future tests.
Fix by returning the receiver:
Proposed fix
func (f *TestLogger) FromContext(_ context.Context) Logger {
- return NewTestLogger()
+ return f
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (f *TestLogger) FromContext(_ context.Context) Logger { | |
| return NewTestLogger() | |
| } | |
| func (f *TestLogger) FromContext(_ context.Context) Logger { | |
| return f | |
| } |
🤖 Prompt for AI Agents
In `@pkg/plugins/log/fake.go` around lines 46 - 48, FromContext currently returns
a brand-new TestLogger, disconnecting it from the original logger and breaking
assertions; change TestLogger.FromContext to return the receiver (retain the
original TestLogger's context) instead of NewTestLogger() so LoggerMiddleware
calls like m.logger.FromContext(ctx).Info(...) operate on the same TestLogger
instance (refer to TestLogger.FromContext and LoggerMiddleware).
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | ||
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | ||
| return m.next.QueryData(ctx, req) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | ||
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | ||
| return m.next.CallResource(ctx, req, sender) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | ||
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | ||
| return m.next.CheckHealth(ctx, req) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | ||
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | ||
| return m.next.CollectMetrics(ctx, req) | ||
| } |
There was a problem hiding this comment.
Nil req dereference will panic.
QueryData, CallResource, CheckHealth, and CollectMetrics access req.PluginContext without a nil check on req. The downstream LoggerMiddleware guards against nil requests (e.g., logger_middleware.go lines 63-65), but this middleware runs before it in the chain, so a nil request causes a panic here first.
Proposed fix (example for QueryData, apply to all four methods)
func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
+ if req == nil {
+ return m.next.QueryData(ctx, req)
+ }
ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext)
return m.next.QueryData(ctx, req)
}
func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error {
+ if req == nil {
+ return m.next.CallResource(ctx, req, sender)
+ }
ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext)
return m.next.CallResource(ctx, req, sender)
}
func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) {
+ if req == nil {
+ return m.next.CheckHealth(ctx, req)
+ }
ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext)
return m.next.CheckHealth(ctx, req)
}
func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) {
+ if req == nil {
+ return m.next.CollectMetrics(ctx, req)
+ }
ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext)
return m.next.CollectMetrics(ctx, req)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | |
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | |
| return m.next.QueryData(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | |
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | |
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | |
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | |
| return m.next.CollectMetrics(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | |
| if req == nil { | |
| return m.next.QueryData(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | |
| return m.next.QueryData(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | |
| if req == nil { | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | |
| if req == nil { | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | |
| if req == nil { | |
| return m.next.CollectMetrics(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | |
| return m.next.CollectMetrics(ctx, req) | |
| } |
🤖 Prompt for AI Agents
In
`@pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go`
around lines 39 - 57, The four middleware methods (QueryData, CallResource,
CheckHealth, CollectMetrics) currently dereference req.PluginContext without
checking req for nil; update each method to first check if req == nil and only
call instrumentContext with req.PluginContext when req is non-nil (otherwise
call instrumentContext with a nil/empty plugin context or leave ctx unchanged),
then forward the call to m.next with the original req and parameters; ensure you
reference and modify the functions QueryData, CallResource, CheckHealth, and
CollectMetrics in contextual_logger_middleware.go.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Improvements